Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release package compatibility #4764

Merged
merged 13 commits into from
May 2, 2023

Conversation

tuffnatty
Copy link
Contributor

@tuffnatty tuffnatty commented Apr 14, 2023

This is a long patch set, and probably it could be better structured. What it gives is:

  • Ubuntu 16.04+ compatible build with Qt 5.15.9 and ICU 72.1
  • Windows builds with Qt 5.15.9
  • MacOS 10.13+ compatible build with Qt 5.15.8
  • MacOS 10.12 compatible build with Qt 5.13.2
  • optional package with embedded Python on MacOS

@pawelsalawa
Copy link
Owner

Impressive piece of work. As it's WIP, I'll hold with mering.

@tuffnatty
Copy link
Contributor Author

@pawelsalawa Generally, I am satisfied with the resulting builds. You can see and test them at https://github.com/tuffnatty/sqlitestudio/actions .

The Linux build has multiplied: now there are 5 workflows you have to run in sequence if running for the first time. The new four workflows build the runner container and SQLiteStudio's bundled dependencies. Probably the dependency workflows should be merged in one (Qt takes an hour, the others take a couple of minutes each) but it has been easier to debug separately so far.
The final Linux release build produces loads of packages using build matrix, but actually only glibc_2.23/Qt 5.15.9 is the one I am suggesting to keep. glibc_2.31/Qt 5.15.2 is basically what we were having lately with the Ubuntu 20.04 runner.

Bundling Python 3.9 on MacOS is a questionable decision. If we bundle it ourselves why not 3.11? Did not try it yet but 3.10 worked well on one occasion. For the slim package, I think it would be better to let user choose their desired Python shared library in configuration and link to it dynamically (discussed in #4662), but if it has not become possible with the recent versions, I would suggest shipping 3 plugin versions.

Keeping the numerous Qt's dependency versions in the MacOS workflow may be boring, but until the distfiles cache is cleared it should not be necessary. Probably can also be automated.

@pawelsalawa
Copy link
Owner

Will the Qt have to be built more than once? I imagine it would be required only if anything changes in Qt, right? (like version, or patches applied)

Python 3.11 - the problem here will be C API or ABI compatibility. I don't remember exactly right now, because it's been a while now since I worked on Python plugin, but I remember that I was surprised, that the API compatibility is not out of box when going with newer versions. It's something worth trying and verifying (if the plugin loads to SQLiteStudio, it should be fine).

@tuffnatty
Copy link
Contributor Author

Yes, the plan is to keep the build cached until Qt version change or new patches. I also think the old build artifacts are occasionally cleaned automatically, and an infrequent scheduled rebuild should help here.

@tuffnatty tuffnatty force-pushed the compatibility-builds branch 2 times, most recently from 4cbd283 to 4854d2a Compare April 15, 2023 14:30
@tuffnatty
Copy link
Contributor Author

tuffnatty commented Apr 15, 2023

@pawelsalawa I have included the small dependencies (ICU, OpenSSL, and Tcl) to the container image, leaving us with just 3 Linux workflows and 12 commits to review.

@pawelsalawa
Copy link
Owner

While I admire the effort and beauty of having various variants of builds (different Qt versions, different glibc versions on Linux), is it really profitable? I mean since glibc_2.23 will work on both older and newer Linux (if I understand it correctly), then is it worth to build the others, which are not as portable? Also if we can have Qt 5.15.9, why keeping 5.15.2?

If there are good reasons, I'm okay with it. Otherwise I think it will be waste of space and GitHub actually does limit it (it charges a fee for everything over 500MB currently stored in all action artifacts). I have a cleanup job in place, but having so many artifacts has significant impact on how long artifacts can be kept.

We can keep all the action code that builds other variations as commented code - just in case.

What do you think?

@pawelsalawa
Copy link
Owner

As I go through the lin_release now, I think that removing the Qt 5.15.2 completely from the workflow would significantly simplify this workflow and - if I'm not mistaken - would enable the Qt 5.15.9 to be included in docker images, thus making it more persistent across subsequent release builds, right?

@tuffnatty
Copy link
Contributor Author

tuffnatty commented Apr 15, 2023

@pawelsalawa Of course it's not directly profitable but helps to ensure the builds are compatible with a variety of environments. I have just removed some extra Linux builds leaving only Xenial/Qt-5.15.9 and Bionic/Qt-5.15.2. The 5.15.2 build uses the same Qt binaries provided by aqtinstall as the old build, so I thought it can probably help to catch any problems with the new build during the transition period. Naturally, removing it will make the code a bit cleaner.
As for the GitHub storage limits, are you sure it applies to public repositories? (runs to clean his artifacts)

@pawelsalawa
Copy link
Owner

@tuffnatty Okay, that makes sense. Let's keep it for now.

And you're right about no limits for public repos. I wasn't aware of that.

@pawelsalawa
Copy link
Owner

There's an issue with builds for Windows with Qt 5.15.9. At least builds I took from your fork. For 32 bit version I get following error when I try to run it:
image
and for 64 bit I got 2 errors one after another:
image
image

Windows builds with Qt 5.15.2 work fine.

If Windows is not your daily OS, I can do the smoke tests (like this one) for you. Not a problem.

@tuffnatty
Copy link
Contributor Author

Well, the Windows builds definitely used to start at one moment, but then something changed... will check soon.

@tuffnatty
Copy link
Contributor Author

Windows builds are working again.

@tuffnatty tuffnatty changed the title WIP: Release package compatibility Release package compatibility Apr 18, 2023
@pawelsalawa
Copy link
Owner

pawelsalawa commented Apr 18, 2023

I confirm that now the Windows builds work.

I see you've removed the WIP prefix. Shall I do one more review and merge, or do you foresee some additional commits?

@tuffnatty
Copy link
Contributor Author

  1. I see a new ICU release but I think it's better to keep a single version on all platforms for now.
  2. You suggested including Qt build to the container build. It's very easy but then Qt will have to be rebuilt when upgrading any of the other dependencies. Not a great problem, of course. I can do it if you wish.

@pawelsalawa
Copy link
Owner

Since we agreed to support more than one Qt version in release packages, I think we should stay with Qt outside of the container. In that case I will have one last look (not now, but soon) and merge.

This is really great piece of work you've done here. Thank you!

@pawelsalawa pawelsalawa merged commit ddbfe26 into pawelsalawa:master May 2, 2023
@pawelsalawa
Copy link
Owner

@tuffnatty Hi, would you mind checking why some Windows builds failed? https://github.com/pawelsalawa/sqlitestudio/actions/runs/4910709807 and https://github.com/pawelsalawa/sqlitestudio/actions/runs/4910827559

You may probably have better/quicker guess. I took a look, but I have no quick answer.

@tuffnatty
Copy link
Contributor Author

@tuffnatty Hi, would you mind checking why some Windows builds failed? https://github.com/pawelsalawa/sqlitestudio/actions/runs/4910709807 and https://github.com/pawelsalawa/sqlitestudio/actions/runs/4910827559

You may probably have better/quicker guess. I took a look, but I have no quick answer.

See PR #4792. I also do not quite understand what was happening there but It got fixed by switching to msys2 bash for compiling.

@tuffnatty tuffnatty deleted the compatibility-builds branch July 10, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants